-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BFD-3663: Populate tags for claim / EOB on ingestion from CCW #2497
Conversation
apps/bfd-model/bfd-model-rif/src/main/java/gov/cms/bfd/model/rif/samhsa/CcwTagKey.java
Outdated
Show resolved
Hide resolved
…if/samhsa/CcwTagKey.java Co-authored-by: aschey-forpeople <[email protected]>
...ine/bfd-pipeline-shared-utils/src/main/java/gov/cms/bfd/pipeline/sharedutils/SamhsaUtil.java
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,706 @@ | |||
- claimClass: CarrierClaim |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clever solution! My only concern is that we're giving up a lot of compile-time safety by invoking all of this dynamically. Alternatively, we could have an adapter class for each claim type - HHASamhsaAdapter
, InpatientSamhsaAdapter
, etc. that would wrap each claim object and implement some interface or inherit from some base class that we could operate on using common methods just like what you've done in these yaml files. It would be a bit more boilerplate than using reflection, but I think it would be a bit less prone to accidental breakage. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If your concern about compile-time safety is that the methods may not exist, then the integration tests should alleviate that -- Every method in the yaml file is exercised. Or did you have a different concern?
The adapter solution would work, but I think it would be a bit more tightly coupled than the yaml solution. What I mean is that if, in the future, we add new SAMHSA fields, we would need only add two lines into the YAML file (column and method).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we leave this code as is and never change it, then yes, the tests will catch it. My main concern with these kinds of things is that it's more difficult to refactor and extend safely in the future. I think the concept here makes sense - with the adapter pattern we can still use getFields()
as you have here and we should only need to add 1-2 lines of code to add the new field there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made some changes using the adapter pattern, like we talked about. I still think reflection is the way to go here; otherwise, we'd have to set method references in the base adapter class, which would require each adapter to pass each of it's relevant methods (since, while the entities share a base class, the base class does not define most of the methods). This can be up to 66 methods in some entities, which defeats the purpose of this streamlining process.
Another idea I've been thinking about is annotating all of the samhsa field get methods in the entities with @samhsa
(or something similar), which would allow us to programmatically grab a list of all of the annotated methods at once, without having to explicitly reference them by name; however, I'm not quite sure how this would work with our generated entities -- it would be a bit more complex than if the entities were hand woven.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Been thinking about this, and I've come to agree with what you have been trying to get at: passing the methods along from the adapter with a Supplier
object may be a better solution for the sake of maintainability Reflection would make the code a lot more compact, but:
- Too easy to make a mistake, and
- I'm not convinced that the execution time of invoking a method with reflection is advisable due to the volume of claims that we process at a time.
case RdaMcsClaim mcsClaim -> { | ||
Optional<List<McsTag>> tags = Optional.of(checkAndProcessMcsClaim(mcsClaim)); | ||
persistTags(tags, entityManager); | ||
public <TClaim> boolean processClaim(TClaim claim, EntityManager entityManager) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need the return value here? Doesn't seem like it's used right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be used in the backfill code, to keep track of how many tags were created.
* @param claim The claim to check. | ||
* @return A list of tag entities to persist. | ||
*/ | ||
private List<HhaTag> checkAndProcessHhaClaim(HHAClaim claim) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this logic can operate on an input of SamhsaAdapterBase
so it doesn't need to be duplicated for each claim type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe? Each tag type would still need to be instantiated individually, so I'm not sure how much value there would be to moving this to the adapter. Can you elaborate on what you're thinking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we would probably need a method called buildTag
or something in the adapter that instantiates the tag object from the claim ID, code, and details, but if we did that, we wouldn't need a separate checkAndProcessXXXClaim
method for each claim type in here and we could get rid of the big switch statement on line 135. I haven't tried it myself, so I could be missing something, but I think that would work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm hoping the adapter can simplify the logic in this file as much as possible so we can eliminate all of the switch statements and confine any duplicated code to the adapters. It won't prevent all the annoying boilerplate, but it at least prevents the need for big switch statements and keeps the main logic separate which I think helps with readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would still need to know which adapter to instantiate; we may be able to move the switch logic to a factory class, but I don't think it will be possible to eliminate it completely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's true. It's just one line to instantiate the adapter though so that's not the end of the world.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored based on this conversation.
JIRA Ticket:
BFD-3663
What Does This PR Do?
Populate SAMHSA tags for claims on ingestion from CCW.
What Should Reviewers Watch For?
If you're reviewing this PR, please check for these things in particular:
What Security Implications Does This PR Have?
Please indicate if this PR does any of the following:
Adds any new software dependencies
Modifies any security controls
Adds new transmission or storage of data
Any other changes that could possibly affect security?
I have considered the above security implications as it relates to this PR. (If one or more of the above apply, it cannot be merged without the ISSO or team security engineer's (
@sb-benohe
) approval.)Validation
Have you fully verified and tested these changes? Is the acceptance criteria met? Please provide reproducible testing instructions, code snippets, or screenshots as applicable.
Unit tests and integration tests have been created.